Skip to content

RDKEMW-19653 : Deprecated DataModel Removal for XRE and XCaliber code from RDKE - #520

Open
madhubabutt wants to merge 2 commits into
developfrom
feature/RDKEMW-19653
Open

RDKEMW-19653 : Deprecated DataModel Removal for XRE and XCaliber code from RDKE#520
madhubabutt wants to merge 2 commits into
developfrom
feature/RDKEMW-19653

Conversation

@madhubabutt

@madhubabutt madhubabutt commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Reason for change: Removal of deprecated XRE/Xcalibur profile infrastructure (USE_XRESRC, XRELIB_FLAG, and all Xcalibur parameters except xconfCheckNow) while preserving minimal xconfCheckNow parameter support.
Test Procedure: Build RDKE with the change, validate Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow functionality, perform stability/reboot tests ensuring no crashes.
Risks: Medium
Priority: P1
Signed-off-by: Tirumala, Madhubabu (Contractor) Madhubabu_Tirumala@comcast.com

Copilot AI review requested due to automatic review settings August 3, 2026 06:39
@madhubabutt
madhubabutt requested a review from a team as a code owner August 3, 2026 06:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes deprecated XRE/Xcalibur data model and build/config wiring from the tr69hostif codebase, aligning RDKE builds and runtime configuration away from the legacy XRE/Xcalibur profile.

Changes:

  • Removes XRE/Xcalibur-specific build conditionals and linkage (XRELIB_FLAG, --enable-xre, -ltr69ProfileXcaliber).
  • Drops Xcalibur/XRE manager mappings from runtime and integration-test configuration.
  • Prunes XRE/Xcalibur-related WALDB data model objects from the TV and generic data model XMLs.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Makefile.am Removes XRELIB_FLAG-based linking and Xcaliber profile library linkage.
configure.ac Removes --enable-xre option, conditional, and XRE substitution variables.
conf/tr69hostIf.conf Removes XRE/Xcalibur object-to-manager mappings.
conf/mgrlist.conf Removes Xcalibur manager entry from the manager list.
src/integrationtest/conf/mgrlist.conf Mirrors mgrlist removal for integration-test configuration.
src/hostif/parodusClient/waldb/data-model/data-model-tv.xml Removes Xcalibur CoPilot object definitions from the TV WALDB model.
src/hostif/parodusClient/waldb/data-model/data-model-generic.xml Removes XRPairing RFC object and large Xcalibur/XRE/TRM sections from the generic WALDB model.

Comment thread src/hostif/parodusClient/waldb/data-model/data-model-generic.xml
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Code Coverage Summary

                                        Total:|74.2%  9594|90.4% 659|    -    0

@madhubabutt
madhubabutt force-pushed the feature/RDKEMW-19653 branch from 3a7de3d to d90824e Compare August 3, 2026 12:50
Copilot AI review requested due to automatic review settings August 3, 2026 12:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:133

  • The XRE client handler is documented as “xconfCheckNow only”, but handleSetMsg still accepts additional Xcalibur parameters under RDKV_TR69 (xreReceiverRestart/devAppRestartRequest). Since the XRE/Xcalibur data model and manager mappings are being removed, these extra branches are now inconsistent and can leave partially-supported/undocumented behavior. Consider removing the RDKV_TR69-only branches so this handler truly only supports xconfCheckNow.
    #ifdef RDKV_TR69
    else if(strcasecmp(stMsgData->paramName,"Device.X_COMCAST-COM_Xcalibur.Client.XRE.xreReceiverRestart") == 0)
    {
        ret = setXreReceiverRestart(stMsgData);
    }

src/hostif/profiles/DeviceInfo/Makefile.am:27

  • This Makefile now has the same MoCA include path listed twice, which is redundant and makes the include list harder to maintain. Remove the duplicate entry.
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:214

  • The xconfCheckNow input validation accepts prefix matches because it uses strncasecmp(..., strlen("TRUE")) / strlen("CANARY") (e.g., "TRUE123" passes). If the intent is to only accept the exact tokens TRUE or CANARY (case-insensitive), use strcasecmp for full-string comparison.
/**
 * @brief Set xconfCheckNow parameter to trigger firmware update check.
 * @param[in] stMsgData Message data containing the parameter value.
 * @return OK on success, NOK on failure.
 */
int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)

Comment thread src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Code Coverage Summary

                                        Total:|68.6%  8203|86.0% 578|    -    0

@madhubabutt
madhubabutt force-pushed the feature/RDKEMW-19653 branch from d90824e to 3678902 Compare August 4, 2026 06:15
Copilot AI review requested due to automatic review settings August 4, 2026 06:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:140

  • handleSetMsg() still references setXreReceiverRestart() and setDevAppRestartRequest() under #ifdef RDKV_TR69, but those functions are not defined anywhere after the XRE/Xcalibur removal. This will fail to compile when RDKV_TR69 is enabled. Since the PR scope says only xconfCheckNow remains, drop these legacy branches (or reintroduce their implementations).
    #ifdef RDKV_TR69
    else if(strcasecmp(stMsgData->paramName,"Device.X_COMCAST-COM_Xcalibur.Client.XRE.xreReceiverRestart") == 0)
    {
        ret = setXreReceiverRestart(stMsgData);
    }
    else if(strcasecmp(stMsgData->paramName,"Device.X_COMCAST-COM_Xcalibur.DevApp.devAppRestartRequest") == 0)
    {
        // xreReceiverRestart uses /lib/rdk/restartReceiver.sh, which sometimes doesn't start receiver back 
        ret = setDevAppRestartRequest(stMsgData);
    }
    #endif

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:259

  • get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() reads at most 9 characters and doesn't populate paramLen/paramtype, which can lead to truncated values and inconsistent TR-069 responses. Prefer reading up to TR69HOSTIFMGR_MAX_PARAM_LEN-1, stripping newline, and setting paramLen + paramtype.
    fscanf(file, "%9s", stMsgData->paramValue);
    fclose(file);
    return OK;

src/hostif/profiles/DeviceInfo/Makefile.am:28

  • AM_CXXFLAGS contains the same MoCA include path twice, which is redundant and makes future maintenance noisier.
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \
$(GLIB_CFLAGS) $(G_THREAD_CFLAGS) $(MOCAMGR_FLAGS) $(PROCPS_CFLAGS) $(YOCTO_FLAG) \

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:159

  • Doc comment has a double period in "device..", which looks accidental and will show up in generated documentation.
 * @return Returns the status of the operation.
 * @retval OK if it is successfully.
 * @retval ERR_INTERNAL_ERROR if not able to get data from the device..
 * @ingroup TR-069HOSTIF_XRECLIENT_REQHANDLER_CLASSES

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Code Coverage Summary

                                        Total:|68.6%  8203|86.0% 578|    -    0

Copilot AI review requested due to automatic review settings August 4, 2026 10:00
@madhubabutt
madhubabutt force-pushed the feature/RDKEMW-19653 branch from 3678902 to bbef606 Compare August 4, 2026 10:00
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Code Coverage Summary

                                        Total:|68.6%  8203|86.0% 578|    -    0

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:133

  • The file header/note says only Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow remains, but handleSetMsg() still accepts other XRE/DevApp control parameters under #ifdef RDKV_TR69 (e.g., ...Client.XRE.xreReceiverRestart, ...DevApp.devAppRestartRequest). If those parameters are deprecated/removed from the data model, this branch should be removed so the handler behavior matches the documented scope.
    #ifdef RDKV_TR69
    else if(strcasecmp(stMsgData->paramName,"Device.X_COMCAST-COM_Xcalibur.Client.XRE.xreReceiverRestart") == 0)
    {
        ret = setXreReceiverRestart(stMsgData);
    }

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:255

  • get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() returns OK without setting paramtype / paramLen, and doesn't clear paramValue before reading. Callers can observe stale buffer contents or a zero length even on success; set hostIf_StringType and paramLen when returning a value.
    if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
        fclose(file);
        return NOK;
    }

src/hostif/profiles/DeviceInfo/Makefile.am:28

  • src/hostif/profiles/DeviceInfo/Makefile.am now has the MoCA include path listed twice consecutively, which is redundant and makes the include list harder to maintain.
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \
$(GLIB_CFLAGS) $(G_THREAD_CFLAGS) $(MOCAMGR_FLAGS) $(PROCPS_CFLAGS) $(YOCTO_FLAG) \

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:211

  • set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() currently uses strncasecmp(..., strlen("TRUE")) / strlen("CANARY"), which accepts prefix values like TRUEjunk. It also writes the raw value to /tmp/xconfchecknow_val even when the input is invalid (write happens before validation). Validate an exact value first and only then persist a canonical value.
int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
    FILE *file = fopen("/tmp/xconfchecknow_val", "w");

@madhubabutt
madhubabutt force-pushed the feature/RDKEMW-19653 branch from bbef606 to 5f28316 Compare August 4, 2026 10:29
Copilot AI review requested due to automatic review settings August 4, 2026 10:29
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Code Coverage Summary

                                        Total:|63.1%  9593|79.7% 659|    -    0

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:215

  • set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() writes the requested value to /tmp before validating it, so invalid inputs still get persisted. It also matches only a prefix (e.g., "TRUE123" passes) and does not set a faultCode for invalid values. Validate first (exact match), set fcInvalidParameterValue on invalid input, then persist the value only on success.
    FILE *file = fopen("/tmp/xconfchecknow_val", "w");
    if (file == NULL) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for write.\n",__FILE__,__FUNCTION__,__LINE__);
        return NOK;
    }

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:255

  • get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() populates paramValue but never sets paramLen. Many other handlers set paramLen after filling paramValue, and callers may rely on it for transport/serialization.
    if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
        fclose(file);
        return NOK;
    }

src/hostif/profiles/DeviceInfo/Makefile.am:27

  • Duplicate include path: DeviceInfo/Makefile.am adds the MoCA include directory twice consecutively. This is redundant and makes flag lists harder to maintain.
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \

docs/api/dml_parameter_list.md:9

  • The access-count summary table is internally inconsistent: 306 readOnly + 543 readWrite = 849, but the Total row says 848.
| readOnly  | 306 | GET only - assert non-empty or expected value |
| readWrite | 543 | GET + SET - verify set/get round-trip |
| **Total** | **848** | |

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:23

  • The file header says only xconfCheckNow remains, but handleSetMsg() still supports additional legacy parameters under RDKV_TR69 (xreReceiverRestart/devAppRestartRequest). Either remove those code paths or update this note so the documented scope matches the actual supported parameters.

This issue also appears in the following locations of the same file:

  • line 211
  • line 251
 * @file hostIf_XREClient_ReqHandler.cpp
 * @brief Minimal handler for Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow parameter.
 * @note All XRE/Xcalibur profile features except xconfCheckNow have been removed.

src/hostif/handlers/include/hostIf_XREClient_ReqHandler.h:23

  • The header note claims only xconfCheckNow is supported, but the implementation still has additional legacy SET parameters under RDKV_TR69. Update the note (or remove the legacy paths) so API docs match behavior.
 * @file hostIf_XREClient_ReqHandler.h
 * @brief Minimal handler for Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow parameter.
 * @note All XRE/Xcalibur profile features except xconfCheckNow have been removed.
 */

Copilot AI review requested due to automatic review settings August 5, 2026 07:10
@madhubabutt
madhubabutt force-pushed the feature/RDKEMW-19653 branch from 5f28316 to 4f2b8db Compare August 5, 2026 07:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:204

  • set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() persists the raw input to /tmp/xconfchecknow_val before validating it, and the current strncasecmp(..., strlen("TRUE")) / strncasecmp(..., strlen("CANARY")) checks accept prefixes (e.g. "TRUE123") even though the error message/documentation says only "TRUE" or "CANARY" are valid. This can leave an invalid value persisted even when returning NOK, and it can trigger the updater for unintended strings.
    FILE *file = fopen("/tmp/xconfchecknow_val", "w");
    if (file == NULL) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for write.\n",__FILE__,__FUNCTION__,__LINE__);
        return NOK;
    }

src/Makefile.am:58

  • WiFi profile support is being removed from the build here (the WITH_WIFI_PROFILE block that defined USE_WIFI_PROFILE and added the WiFi include path was deleted), but configure.ac still exposes --enable-wifi / WITH_WIFI_PROFILE and the default runtime config still maps Device.WiFi to wifiMgr (e.g. conf/tr69hostIf.conf:10, conf/mgrlist.conf:9). As-is, --enable-wifi becomes misleading (WiFi libs may build, but the main binary no longer compiles/links WiFi handler support) and the shipped configs reference a manager that will never be registered.
if WITH_DHCP_PROFILE
AM_CXXFLAGS += -DUSE_DHCPv4_PROFILE
AM_CXXFLAGS += -I$(top_srcdir)/src/hostif/profiles/DHCPv4
endif

docs/integration/build-setup.md:33

  • The build documentation still states that --enable-wifi enables WiFi profile support, but this PR removes WiFi handler/profile wiring from the build (e.g. WITH_WIFI_PROFILE blocks removed from src/Makefile.am and src/hostif/handlers/Makefile.am). This flag description should be updated to match the new behavior (removed/unsupported, or clarify what it still builds).
The top-level `configure.ac` currently exposes feature toggles including:

| Flag | Effect |
|------|--------|
| `--enable-moca` / `--enable-moca2` | Enable MoCA profile support |
| `--enable-DHCPv4` | Enable DHCPv4 profile support |
| `--enable-StorageService` | Enable StorageService profile support |
| `--enable-InterfaceStack` | Enable InterfaceStack profile support |
| `--enable-notification` | Enable value-change notification support |

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3766

  • set_xRDKCentralComRFC() no longer handles Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.LXC.XRE.Enable (XRE container RFC), but the DeviceInfo gtest suite still asserts this parameter returns OK (e.g. DeviceInfo/gtest/gtest_main.cpp:3608) and run_ut.sh still creates /opt/XRE_container_enable. This will cause the existing unit test to fail (and leaves stale test scaffolding).
    else if (strcasecmp(stMsgData->paramName,LE_RFC_ENABLE) == 0)
    {
        ret = set_xRDKCentralComRFCLoudnessEquivalenceEnable(stMsgData);
    }
    else if (strcasecmp(stMsgData->paramName,DAB_RFC_ENABLE) == 0)
    {
        ret = set_xRDKCentralComDABRFCEnable(stMsgData);
    }
    else if (strcasecmp(stMsgData->paramName,RFC_CTL_RETRIEVE_NOW) == 0)
    {
        ret = set_xRDKCentralComRFCRetrieveNow(stMsgData);

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:246

  • xconfCheckNow set/get behavior is now part of the always-built XREClient handler, and get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() has new error-handling (fscanf return check). There are gtest suites in the repo, but no tests currently exercise Device.X_COMCAST-COM_Xcalibur.Client.xconfCheckNow (set/get, invalid input, missing file, read failure), so regressions here will be untested.
int get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
    FILE *file = fopen("/tmp/xconfchecknow_val", "r");
    if (file == NULL) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for read.\n",__FILE__,__FUNCTION__,__LINE__);
        return NOK;
    }
    if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
        fclose(file);
        return NOK;
    }
    fclose(file);
    return OK;

Copilot AI review requested due to automatic review settings August 5, 2026 07:16
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Code Coverage Summary

                                        Total:|56.6%  9566|69.9% 658|    -    0

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:40

  • m_mutex is never statically initialized, and getLock() currently calls g_mutex_init() every time it locks (re-initializing a mutex that may already be in use is undefined and can break locking). Initialize the static mutex once via G_MUTEX_INIT so getLock() only needs to lock/unlock it.
XREClientReqHandler* XREClientReqHandler::pInstance = NULL;
GMutex XREClientReqHandler::m_mutex;

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:88

  • getLock() re-initializes the mutex each time it is called. With m_mutex statically initialized, getLock() should only lock the mutex (re-initializing a mutex that may be locked by another thread is undefined behavior).
void XREClientReqHandler::reset()
{
    getLock();
    RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s:%s] Entering..\n", __FUNCTION__, __FILE__);
    RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s:%s] Exiting..\n", __FUNCTION__, __FILE__);

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:244

  • get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() returns NOK on file open/read failures but does not set a TR-069 fault code. Other handlers typically set stMsgData->faultCode (e.g., fcInternalError) so callers can distinguish internal failures.
    FILE *file = fopen("/tmp/xconfchecknow_val", "r");
    if (file == NULL) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for read.\n",__FILE__,__FUNCTION__,__LINE__);
        return NOK;
    }
    if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
        fclose(file);
        return NOK;
    }

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:200

  • When /tmp/xconfchecknow_val cannot be opened/written, the function returns NOK but does not set stMsgData->faultCode, and fprintf() write errors are ignored. Setting fcInternalError and checking fprintf() makes failures visible to TR-069 callers.

This issue also appears on line 235 of the same file.

int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
    FILE *file = fopen("/tmp/xconfchecknow_val", "w");

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:225

  • v_secure_system() returns the child exit status (0 on success, non-zero on failure). The current check only treats -1 as failure, and invalid inputs are written to /tmp/xconfchecknow_val before validation. At minimum, treat any non-zero command exit as failure and roll back the persisted value for invalid input.
    return OK;

Comment thread src/hostif/profiles/DeviceInfo/Device_DeviceInfo.h
Copilot AI review requested due to automatic review settings August 5, 2026 10:51
@madhubabutt
madhubabutt force-pushed the feature/RDKEMW-19653 branch from bdaa63f to 2a065d6 Compare August 5, 2026 10:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.

Suppressed comments (5)

src/hostif/handlers/Makefile.am:112

  • configure.ac still supports WITH_WIFI_PROFILE/USE_WIFI_PROFILE, and the codebase still includes hostIf_WiFi_ReqHandler.cpp, but this Makefile no longer adds that source file conditionally. With --enable-wifi builds, this will typically lead to undefined references (WiFiClientReqHandler) and/or Device.WiFi manager initialization failures.
if WITH_MOCA_PROFILE
libMsgHandlers_la_SOURCES +=    src/hostIf_MoCAClient_ReqHandler.cpp
endif

if WITH_DHCP_PROFILE

src/Makefile.am:52

  • This Makefile dropped the WITH_WIFI_PROFILE CXXFLAGS block, but configure.ac still provides --enable-wifi and sets WIFI_PROFILE_FLAG/SUBDIRS_WIFI. As-is, enabling WiFi won’t propagate USE_WIFI_PROFILE or the WiFi include path into the main build, which can break compilation and silently disable Device.WiFi support.
if WITH_MOCA_PROFILE
NEXUS_LIB = -lnexus
AM_CXXFLAGS += -I$(top_srcdir)/src/hostif/profiles/moca
else
NEXUS_LIB =

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:90

  • XREClientReqHandler::getLock() currently calls g_mutex_init() on the static mutex every time it takes the lock. Reinitializing a mutex that may be in use is undefined behavior and can lead to deadlocks/crashes under concurrent access. This should be changed to initialize the mutex once (e.g., in init() or via g_once_init_enter/leave) and then only lock/unlock thereafter.
{
    getLock();
    RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s:%s] Entering..\n", __FUNCTION__, __FILE__);
    RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s:%s] Exiting..\n", __FUNCTION__, __FILE__);
    releaseLock();
}

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:246

  • get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() returns OK after reading the stored value but never updates stMsgData->paramtype or stMsgData->paramLen. Callers can observe stale metadata from prior requests. Populate these fields on success (string type + length).
    if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
        fclose(file);
        return NOK;
    }
    fclose(file);
    return OK;

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:200

  • set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() writes stMsgData->paramValue to /tmp/xconfchecknow_val before validating it. For invalid inputs, the function returns NOK but still persists the invalid value, which can cause subsequent GETs to return an unsupported value. Consider validating first, setting faultCode=fcInvalidParameterValue on invalid input, and only then persisting/running the script.
int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
    FILE *file = fopen("/tmp/xconfchecknow_val", "w");

Comment thread src/hostif/handlers/Makefile.am
Comment thread src/Makefile.am
Copilot AI review requested due to automatic review settings August 5, 2026 10:57
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Code Coverage Summary

                                        Total:|73.9%  9566|90.4% 658|    -    0

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/hostif/profiles/DeviceInfo/Makefile.am:28

  • DeviceInfo/Makefile.am now has the same MoCA include path twice in AM_CXXFLAGS (lines 26–27). This is redundant and makes future flag edits more error-prone.
-I$(top_srcdir)/src/hostif/handlers/include \
-I$(top_srcdir)/src/hostif/handlers/src \
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \
$(GLIB_CFLAGS) $(G_THREAD_CFLAGS) $(MOCAMGR_FLAGS) $(PROCPS_CFLAGS) $(YOCTO_FLAG) \

src/hostif/handlers/Makefile.am:38

  • handlers/Makefile.am still includes -I$(top_srcdir)/src/hostif/profiles/moca twice (lines 25 and 36). Removing the duplicate keeps AM_CXXFLAGS easier to maintain.
	-I$(top_srcdir)/src/hostif/profiles/InterfaceStack \
	-I$(top_srcdir)/src/hostif/profiles/StorageService \
	-I$(PKG_CONFIG_SYSROOT_DIR)$(includedir)/rdk/iarmmgrs/sysmgr \
	-I$(top_srcdir)/src/hostif/profiles/moca \
	-I./include $(GLIB_CFLAGS) $(G_THREAD_CFLAGS) \
	$(MOCAMGR_FLAGS) $(SOUP_CFLAGS) $(MOCA2_FLAG) \

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:241

  • get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() reads the persisted value with fscanf("%9s", ...), which truncates anything longer than 9 bytes and doesn’t update paramLen. Since the setter writes the full paramValue, this can break set/get round-trips and confuse callers.
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for read.\n",__FILE__,__FUNCTION__,__LINE__);
        return NOK;
    }
    if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);

@madhubabutt
madhubabutt force-pushed the feature/RDKEMW-19653 branch from 39913e5 to 9958564 Compare August 6, 2026 07:14
Copilot AI review requested due to automatic review settings August 6, 2026 07:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/Makefile.am:55

  • WITH_WIFI_PROFILE support appears to have been removed from the build flags, but the project still has a WiFi profile (configure.ac defines WIFI_PROFILE_FLAG and conf files reference Device.WiFi=wifiMgr). As-is, enabling WiFi via configure will no longer define USE_WIFI_PROFILE or add the WiFi include path, so WiFi manager code won’t compile/route correctly.

This issue also appears on line 138 of the same file.

if WITH_DHCP_PROFILE

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:204

  • set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() writes the provided value to /tmp/xconfchecknow_val before validating it. If the input is invalid, the function returns NOK but leaves the persisted state set to the invalid value, which can break subsequent GETs and observability.

This issue also appears on line 242 of the same file.

int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
    FILE *file = fopen("/tmp/xconfchecknow_val", "w");
    if (file == NULL) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for write.\n",__FILE__,__FUNCTION__,__LINE__);

src/Makefile.am:140

  • WITH_WIFI_PROFILE is no longer linked into the tr69hostif binary. Since the WiFi profile library still exists (profiles/wifi/libhostIfWiFi.la) and the default configs still map Device.WiFi to wifiMgr, the resulting binary will miss WiFi support even when configured.
AM_CXXFLAGS += -I$(top_srcdir)/src/hostif/parodusClient/pal 
AM_CXXFLAGS += -I$(top_srcdir)/src/hostif/parodusClient/waldb
tr69hostif_LDADD += $(top_builddir)/src/hostif/parodusClient/waldb/libwaldb.la

src/hostif/handlers/Makefile.am:39

  • WIFI profile include/define flags were removed from handler compilation. When WiFi is enabled, handler sources that include hostIf_WiFi_ReqHandler.h rely on USE_WIFI_PROFILE and the wifi include path; without these, WiFi handler support cannot be built even though configure.ac still exposes WITH_WIFI_PROFILE/WIFI_PROFILE_FLAG.
	-I./include $(GLIB_CFLAGS) $(G_THREAD_CFLAGS) \
	$(MOCAMGR_FLAGS) $(SOUP_CFLAGS) $(MOCA2_FLAG) \
	$(XRDK_SDCARD_PROFILE_FLAG) $(XRDK_EMMC_PROFILE_FLAG) \

src/hostif/handlers/Makefile.am:114

  • WiFi handler source is no longer added to libMsgHandlers when WITH_WIFI_PROFILE is enabled, so wifiMgr routing (Device.WiFi) will fail at runtime and/or link-time if other code expects WiFiReqHandler symbols.
if WITH_MOCA_PROFILE
libMsgHandlers_la_SOURCES +=    src/hostIf_MoCAClient_ReqHandler.cpp
endif

if WITH_DHCP_PROFILE
libMsgHandlers_la_SOURCES +=	src/hostIf_DHCPv4Client_ReqHandler.cpp
endif

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:248

  • get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() returns OK without populating paramtype/paramLen. Other handlers consistently set these fields, and leaving them stale can cause incorrect serialization/processing of the response.
    if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
        fclose(file);
        return NOK;
    }
    fclose(file);
    return OK;

Copilot AI review requested due to automatic review settings August 6, 2026 07:23
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Code Coverage Summary

                                        Total:|73.9%  9566|90.4% 658|    -    0

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/hostif/profiles/DeviceInfo/Makefile.am:27

  • The same MoCA include directory is listed twice in AM_CXXFLAGS, which is redundant and makes future edits noisier.
AM_CXXFLAGS = -I$(top_srcdir)/src/hostif/include \
-I$(top_srcdir)/src/hostif/handlers/include \
-I$(top_srcdir)/src/hostif/handlers/src \
-I$(top_srcdir)/src/hostif/profiles/moca \
-I$(top_srcdir)/src/hostif/profiles/moca \

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:202

  • set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() currently opens/writes /tmp/xconfchecknow_val before validating the input and uses prefix matching via strncasecmp+strlen (so values like "TRUEFOO" would be accepted). This can persist invalid/failed SET values and trigger the upgrader unexpectedly. Consider (1) validating exact allowed values, (2) running the script, and only then (3) persisting the value on success.
int set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow(HOSTIF_MsgData_t *stMsgData)
{
    FILE *file = fopen("/tmp/xconfchecknow_val", "w");

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:246

  • get_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() returns OK/NOK but does not populate stMsgData->paramtype or stMsgData->paramLen, which other handlers rely on for correct serialization/logging. Also, fscanf("%9s") truncates the stored value and stops at whitespace; using fgets and trimming newline is safer and preserves the full stored string.
    if (fscanf(file, "%9s", stMsgData->paramValue) != 1) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error reading file.\n",__FILE__,__FUNCTION__,__LINE__);
        fclose(file);
        return NOK;
    }

Copilot AI review requested due to automatic review settings August 7, 2026 09:08
@madhubabutt
madhubabutt force-pushed the feature/RDKEMW-19653 branch from 4c4a596 to 5ddc18e Compare August 7, 2026 09:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/hostif/handlers/src/hostIf_XREClient_ReqHandler.cpp:206

  • set_Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow() persists stMsgData->paramValue to /tmp/xconfchecknow_val before validating that the input is one of the supported tokens (TRUE/CANARY). If an invalid value is set, the function returns NOK but the invalid value still gets written, which can cause later GETs to return an unsupported value and makes the failure non-atomic. Validate first, then write the file only for valid inputs.
    FILE *file = fopen("/tmp/xconfchecknow_val", "w");
    if (file == NULL) {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF, "[%s:%s:%d]Device_X_COMCAST_COM_Xcalibur_Client_xconfCheckNow: Error opening file for write.\n",__FILE__,__FUNCTION__,__LINE__);
        return NOK;
    }

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Code Coverage Summary

                                        Total:|73.9%  9566|90.4% 658|    -    0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants